[16192] fix notification lost #3087

iuhilnehc-ynos · 2022-11-14T07:48:54Z

Signed-off-by: Chen Lihui lihui.chen@sony.com

Description

Please refer to ros2/rclcpp#2039 (comment).

Contributor Checklist

Reviewer Checklist

Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fujitatomoya

this looks good to me.

MiguelCompany · 2022-11-14T18:08:53Z

@iuhilnehc-ynos Thank you for your contribution. Changes LGTM, but we'll need to add a regression test before merging this.

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos · 2022-11-15T08:44:34Z

@MiguelCompany @fujitatomoya

I have added a regression test. The test log shows that I need 176070(a random number) times to run the test to output an error in my local machine if not applying the patch.

[16:25:12 OptiPlex-7080] /home/chenlh/Projects/ROS2/eProsima/build/fastrtps/test/unittest/dds/core/condition  
chenlh condition $ mytest ./WaitSetImplTests --gtest_filter="*fix_wait_notification_lost"

...

Times 176070
Note: Google Test filter = *fix_wait_notification_lost
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from WaitSetImplTests
[ RUN      ] WaitSetImplTests.fix_wait_notification_lost
/home/chenlh/Projects/ROS2/eProsima/src/fastrtps/test/unittest/dds/core/condition/WaitSetImplTests.cpp:232: Failure
Expected equality of these values:
  ReturnCode_t::RETCODE_OK
    Which is: 0
  ret
    Which is: 4-byte object <01-00 00-00>
terminate called without an active exception

Do you have any suggestions about the regression test?

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fujitatomoya · 2022-11-15T16:59:09Z

@iuhilnehc-ynos

Do you have any suggestions about the regression test?

not really, sorry 😢

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

MiguelCompany · 2022-11-16T07:30:07Z

@richiprosima Please test this

MiguelCompany · 2022-11-16T07:56:35Z

@iuhilnehc-ynos It seems this PR is introducing possible deadlocks. At least that is what the TSan job is complaining about. You can get the report from here.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2022-11-16T09:16:04Z

@iuhilnehc-ynos I think I fixed the deadlock on 8c60364. Let's wait for the TSan job.

MiguelCompany · 2022-11-16T09:17:25Z

@richiprosima Please test this

iuhilnehc-ynos · 2022-11-16T10:02:56Z

@MiguelCompany

I just analyzed the log, but you have already fixed the potential deadlock. Thank you.

MiguelCompany · 2022-11-16T12:22:10Z

@MiguelBarro This has a deadlock report on code related with ReadConditions. Would you mind taking a look?

MiguelBarro · 2022-11-18T13:39:42Z

The fix consists on assuring no notification will take place on predicate evaluation by taken the mutex during notifications (WaitSetImpl::wake_up changes).
Unfortunately, that introduces an ABBA deadlock on intraproccess scenarios:

Waiting thread:
- A waitset mutex (wait())
- B reader mutex (retrieving trigger value)
Writing thread:
- B reader mutex (StatefulReader::processDataMsg directly called from writer)
- A waitset mutex (notification to the Conditions implies wakeup)

A workaround may be to introduce an atomic_uint notification counter in the WaitSetImpl. This will be updated on wake up avoding taking the mutex. The wait predicate must check the counter and decide to reevaluate.
Another alternative is avoid taking the reader mutex on Condition notification (there is a devoted mutex for the condition collection). The issue here is that the reader mutex is recursive and taken upstream in the stack which makes it a costly refactor.
Surely I came up with a better solution over the weekend.

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

fujitatomoya · 2022-12-07T18:12:23Z

@MiguelCompany @MiguelBarro anything we need to do on our side? friendly ping.

fujitatomoya · 2023-01-03T20:23:09Z

friendly ping, this can resolve a couple of issues.

MiguelCompany · 2023-01-05T08:20:09Z

@richiprosima Please test this

MiguelCompany · 2023-01-05T11:13:51Z

@richiprosima Please test windows

MiguelCompany · 2023-01-05T11:31:34Z

@Mergifyio backport 2.8.x 2.7.x 2.6.x

mergify · 2023-01-05T11:32:08Z

backport 2.8.x 2.7.x 2.6.x

✅ Backports have been created

#3190 [16192] fix notification lost (backport #3087) has been created for branch 2.8.x
#3191 [16192] fix notification lost (backport #3087) has been created for branch 2.7.x
#3192 [16192] fix notification lost (backport #3087) has been created for branch 2.6.x

iuhilnehc-ynos · 2023-01-05T13:33:53Z

I am sorry about the comment ros2/rmw_fastrtps#650 (comment).
I tested with commit 8c60364 before.

If using 34a8f8d, the wait forever issue in ros2/rmw_fastrtps#650 still exists. (just confirmed.)

I was thinking you will use #3098, which can re-check the condition every 500ms.

MiguelCompany · 2023-01-05T15:00:29Z

@iuhilnehc-ynos Thank you for checking with 34a8f8d.

We will then update this PR to aa260da and close #3098.

Would you mind checking ros2/rmw_fastrtps#650 with aa260da to verify it solves the issue?

MiguelCompany · 2023-01-05T15:02:00Z

@richiprosima Please test this

MiguelCompany · 2023-01-05T20:24:44Z

@richiprosima Please test Linux

iuhilnehc-ynos · 2023-01-06T02:29:09Z

Would you mind checking ros2/rmw_fastrtps#650 with aa260da to verify it solves the issue?

I believe this PR with aa260da can fix the wait forever issue, but it leads to another issue (notification is not triggered in time) as below

[INFO] [1672971861.484102416] [sender]: Sending tick: '6357'...              // timer period is set with 10ms
[INFO] [1672971861.493948311] [sender]: Sending tick: '6358'...
[INFO] [1672971861.504098546] [sender]: Sending tick: '6359'...
[INFO] [1672971861.513917624] [sender]: Sending tick: '6360'...
[INFO] [1672971861.523958969] [sender]: Sending tick: '6361'...
[INFO] [1672971861.533877038] [sender]: Sending tick: '6362'...
[INFO] [1672971862.034249728] [sender]: Sending tick: '6363'...             // it costs about 500ms to trigger the timer callback.

* fix notification lost Signed-off-by: Chen Lihui <lihui.chen@sony.com> * add a regression test Signed-off-by: Chen Lihui <lihui.chen@sony.com> * rename a variable name and update comments Signed-off-by: Chen Lihui <lihui.chen@sony.com> * fix uncrustify issue Signed-off-by: Chen Lihui <lihui.chen@sony.com> * make the regression test better Signed-off-by: Chen Lihui <lihui.chen@sony.com> * fix uncrustify Signed-off-by: Chen Lihui <lihui.chen@sony.com> * Refs #16192. Fix deadlock on WaitSetImpl. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs 16192. Workaround for deadlock and notify pessimization Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16192. Mandatory logic fix Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16192. linter Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs #16192. Catch up any notification lost Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16192. Avoid predicate evaluation on spurious wakeups. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16192. Fixing gcc warnings Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16192. Allowing wait without previous notification Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Signed-off-by: Chen Lihui <lihui.chen@sony.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Barro <miguelbarro@eprosima.com> (cherry picked from commit a9e49cd)

This reverts commit a9e49cd.

MiguelCompany · 2023-01-09T08:58:58Z

@iuhilnehc-ynos The browser showed me your commit after merging. We're reverting the merge on #3193.

I thought about this a bit more during the weekend, and decided a new approach, based on your original solution.

After the revert is done, I will create a new PR with the changes on this branch. Could you check aginst that branch?

This reverts commit a9e49cd.

iuhilnehc-ynos · 2023-01-09T09:37:36Z

After the revert is done, I will create a new PR with the changes on this branch. Could you check aginst that branch?

It seems there is some compilation issue on that branch. Maybe you are working on it, I'll check it based on your new PR.

MiguelCompany · 2023-01-09T10:54:47Z

It seems there is some compilation issue on that branch. Maybe you are working on it, I'll check it based on your new PR.

@iuhilnehc-ynos Yeah, sorry about that. It should be fixed now. I have also created #3195 with the backport to 2.6.x, so we can have the fix on Humble too.

fix notification lost

744524a

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos requested a review from MiguelCompany November 14, 2022 08:40

fujitatomoya previously approved these changes Nov 14, 2022

View reviewed changes

add a regression test

db53f1a

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

iuhilnehc-ynos dismissed fujitatomoya’s stale review via db53f1a November 15, 2022 08:44

Chen Lihui added 2 commits November 15, 2022 16:57

rename a variable name and update comments

f1ce5e2

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fix uncrustify issue

a92143e

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Chen Lihui added 2 commits November 16, 2022 10:24

make the regression test better

3e089ff

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

fix uncrustify

764823a

Signed-off-by: Chen Lihui <lihui.chen@sony.com>

MiguelCompany changed the title ~~fix notification lost~~ [16192] fix notification lost Nov 16, 2022

Refs #16192. Fix deadlock on WaitSetImpl.

8c60364

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

mergify bot mentioned this pull request Nov 19, 2022

dummy pr to test deadlock solutions #3098

Closed

13 tasks

Miguel Barro added 7 commits November 19, 2022 16:40

Refs 16192. Workaround for deadlock and notify pessimization

3d03c30

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 16192. Mandatory logic fix

0681786

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 16192. linter

34a8f8d

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs #16192. Catch up any notification lost

eb389f6

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 16192. Avoid predicate evaluation on spurious wakeups.

90d02d3

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 16192. Fixing gcc warnings

b95d811

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

Refs 16192. Allowing wait without previous notification

aa260da

Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>

iuhilnehc-ynos mentioned this pull request Dec 20, 2022

Deadlock when using MultiThreadedExecutor ros2/rmw_fastrtps#650

Closed

MiguelCompany merged commit a9e49cd into eProsima:master Jan 6, 2023

mergify bot mentioned this pull request Jan 6, 2023

[16192] fix notification lost (backport #3087) #3190

Closed

mergify bot mentioned this pull request Jan 6, 2023

[16192] fix notification lost (backport #3087) #3191

Closed

mergify bot mentioned this pull request Jan 6, 2023

[16192] fix notification lost (backport #3087) #3192

Closed

MiguelCompany added a commit that referenced this pull request Jan 9, 2023

Revert "Fix waitset notification (#3087)"

5452591

This reverts commit a9e49cd.

MiguelCompany mentioned this pull request Jan 9, 2023

Revert "Fix waitset notification (#3087)" #3193

Merged

7 tasks

MiguelCompany added a commit that referenced this pull request Jan 9, 2023

Revert "Fix waitset notification (#3087)" (#3193)

e4c5354

This reverts commit a9e49cd.

MiguelCompany mentioned this pull request Jan 9, 2023

[16192] Fix notification lost #3194

Merged

11 tasks

MichaelOrlov mentioned this pull request Mar 17, 2023

👨‍🌾 Regression in test_play_{timing,services}__rmw_{rmw_vendor} on the buildfarm jobs ros2/rosbag2#862

Open

fujitatomoya mentioned this pull request Apr 4, 2023

SharedFuture from async_send_request never becomes valid ros2/rclcpp#2039

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16192] fix notification lost #3087

[16192] fix notification lost #3087

iuhilnehc-ynos commented Nov 14, 2022 •

edited

Loading

fujitatomoya left a comment

MiguelCompany commented Nov 14, 2022

iuhilnehc-ynos commented Nov 15, 2022 •

edited

Loading

fujitatomoya commented Nov 15, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

iuhilnehc-ynos commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelBarro commented Nov 18, 2022 •

edited

Loading

fujitatomoya commented Dec 7, 2022

fujitatomoya commented Jan 3, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

mergify bot commented Jan 5, 2023 •

edited

Loading

iuhilnehc-ynos commented Jan 5, 2023 •

edited

Loading

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

iuhilnehc-ynos commented Jan 6, 2023 •

edited

Loading

MiguelCompany commented Jan 9, 2023

iuhilnehc-ynos commented Jan 9, 2023

MiguelCompany commented Jan 9, 2023

[16192] fix notification lost #3087

[16192] fix notification lost #3087

Conversation

iuhilnehc-ynos commented Nov 14, 2022 • edited Loading

Description

Contributor Checklist

Reviewer Checklist

fujitatomoya left a comment

Choose a reason for hiding this comment

MiguelCompany commented Nov 14, 2022

iuhilnehc-ynos commented Nov 15, 2022 • edited Loading

fujitatomoya commented Nov 15, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

iuhilnehc-ynos commented Nov 16, 2022

MiguelCompany commented Nov 16, 2022

MiguelBarro commented Nov 18, 2022 • edited Loading

fujitatomoya commented Dec 7, 2022

fujitatomoya commented Jan 3, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

mergify bot commented Jan 5, 2023 • edited Loading

✅ Backports have been created

iuhilnehc-ynos commented Jan 5, 2023 • edited Loading

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

MiguelCompany commented Jan 5, 2023

iuhilnehc-ynos commented Jan 6, 2023 • edited Loading

MiguelCompany commented Jan 9, 2023

iuhilnehc-ynos commented Jan 9, 2023

MiguelCompany commented Jan 9, 2023

iuhilnehc-ynos commented Nov 14, 2022 •

edited

Loading

iuhilnehc-ynos commented Nov 15, 2022 •

edited

Loading

MiguelBarro commented Nov 18, 2022 •

edited

Loading

mergify bot commented Jan 5, 2023 •

edited

Loading

iuhilnehc-ynos commented Jan 5, 2023 •

edited

Loading

iuhilnehc-ynos commented Jan 6, 2023 •

edited

Loading